Closes #1317: Add filepath to the start event#1874
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces filepath tracking throughout the HTML hinting process by updating the verify and parse methods and the Block interface. Review feedback highlights that the current modification to the verify method signature is a breaking change for the public API; it is recommended to move filepath to the end of the argument list as an optional parameter to maintain backward compatibility. Further suggestions include making filepath optional in the Block interface to avoid type inconsistencies and providing a default value in the parse method.
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1317 by threading the source filepath through the linting pipeline so that it is exposed on the parser's start event. The CLI's hintFile passes the file path into HTMLHint.verify, which forwards it to HTMLParser.parse, which then includes it on the first start event payload.
Changes:
- Added a required
filepathparameter toHTMLHint.verify, inserted before the optionalrulesetargument. - Added a required
filepathparameter toHTMLParser.parseand a requiredfilepathfield on theBlockinterface; the value is fired with thestartevent. - Updated
hintFilein the CLI to forwardfilepathintoHTMLHint.verify.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/core/core.ts | Adds filepath parameter to verify and forwards it to parser.parse. |
| src/core/htmlparser.ts | Adds filepath to Block interface and to the start event payload; makes the new parse parameter required. |
| src/cli/htmlhint.ts | Passes the file path from hintFile into HTMLHint.verify. |
Comments suppressed due to low confidence (1)
src/core/htmlparser.ts:83
- No test has been added that asserts the new
filepathfield is delivered on thestartevent, even though the rest oftest/htmlparser.spec.jsextensively covers parser events. Given the linked issue #1317 is specifically about exposing the file path on start, a regression test (e.g. spying on thestartevent and assertingevent.filepathequals the value passed toparse) would help guard against future regressions.
this.fire('start', {
pos: 0,
line: 1,
col: 1,
filepath: filepath,
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coliff which path should I take? I assume a backwards-compatible change is the way to go? I went the Gemini-Route for now. |
commodis
force-pushed
the
1317-file-location
branch
from
eb6f4a5 to
fce04e8
Compare
commodis
force-pushed
the
1317-file-location
branch
from
fce04e8 to
8c6504a
Compare
|
@coliff how am I supposed to pass the https://github.com/htmlhint/HTMLHint/actions/runs/25910221464/job/76156669308?pr=1874 edit: ah it seems I have to commit the build - I see |
commodis
force-pushed
the
1317-file-location
branch
from
8c6504a to
2395343
Compare
|
@coliff I dont think I can pass CodeQL tests? Its reporting code I did not change - as far as I can see. |
commodis
force-pushed
the
1317-file-location
branch
from
2395343 to
a44eddd
Compare
|
@coliff is there anything left I can improve? |
|
I've been a bit busy recently but will try and properly review soon. I did look into the CodeQL issues - and they aren't your fault and unrelated to your changes, so don't worry about them :-) |
I am not well versed in Typescript, but I hope this PR suffices.
Short description of what this resolves
It allows rules to implement a custom filter based on the linted filenames.
Proposed changes
It adds the filepath to the start-event.